Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render drop shadow for active window #58

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

weihsinyeh
Copy link
Collaborator

@weihsinyeh weihsinyeh commented Oct 6, 2024

Use Pascal's Triangle to approximate a 5x5 Gaussian Kernel, and apply Gaussian blur on the twin_pixmap_t to implement the blurring technique.

See: #34

Summary by Bito

This PR introduces configurable drop shadow effects for active windows using an optimized stack blur algorithm. The implementation includes customizable shadow offsets, blur radius, and color settings through Kconfig. The changes focus on window rendering and pixmap handling, with performance optimizations in the blur implementation.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 3

include/twin.h Outdated Show resolved Hide resolved
src/draw.c Outdated Show resolved Hide resolved
src/draw.c Outdated Show resolved Hide resolved
@weihsinyeh
Copy link
Collaborator Author

weihsinyeh commented Oct 6, 2024

The idea behind #34 is to implement drop shadow, one of the most popular visual effects in modern window systems and compositing window managers. The effect should be applied at the window management level, not to background images, meaning the background image should not be blurred. The graphical effect of a visually elevated shape with a shadow underneath the window frame should occur transparently.

Because I'm still unsure where to place the blur operation, whether to implement the blur in '_twin_widget_queue_paint()', as this function handles the rendering order of each widget object. It would need to read the corresponding pixel block currently on the screen during rendering to blur the current background. Or should I implement this operation when each widget's TwinEventPaint occurs? So I first try to implement this function on the background to see its effect.

@jserv
Copy link
Contributor

jserv commented Oct 6, 2024

Or should I implement this operation when each widget's TwinEventPaint occurs? So I first try to implement this function on the background to see its effect.

Alternatively, you can write an image viewer with drop shadow functionality to evaluate performance and visual effects in the initial stage. Once you have a better understanding of the window manager events and their flow, we can proceed with integrating it into the window system.

See MDN: drop-shadow

@jserv
Copy link
Contributor

jserv commented Oct 6, 2024

Alternatively, you can write an image viewer with drop shadow functionality to evaluate performance and visual effects in the initial stage.

For performance evaluation, you may consider the code mentioned in #55 (comment) .

@weihsinyeh
Copy link
Collaborator Author

I haven't applied stack blur to the drop shadow yet, because I don't know how to access the current screen's topmost pixel map. I want to apply the stack blur to a specific region of the topmost pixel map that overlaps with the shadow and then use this as the new shadow. The topmost pixel map is efficiently updated by refreshing only the damaged areas.

apps/multi.c Outdated Show resolved Hide resolved
@weihsinyeh weihsinyeh force-pushed the drop_shadow branch 2 times, most recently from a78fc51 to 3433384 Compare November 13, 2024 13:44
@jserv
Copy link
Contributor

jserv commented Nov 13, 2024

Compilation error/warning reported by Clang:

src/window.c:72:13: error: non-void function 'twin_window_create' should return a value [-Wreturn-type]
   72 |             return;
      |             ^
src/window.c:65:17: warning: left operand of comma operator has no effect [-Wunused-value]
   65 |         window->shadow_offset_x, window->shadow_offset_y = 50, 50;
      |         ~~~~~~  ^~~~~~~~~~~~~~~
src/window.c:65:64: warning: expression result unused [-Wunused-value]
   65 |         window->shadow_offset_x, window->shadow_offset_y = 50, 50;
      |                    

@jserv
Copy link
Contributor

jserv commented Nov 13, 2024

I am unsure how to validate this. Could you provide the relevant procedures or methods along with the expected behavior?

@jserv
Copy link
Contributor

jserv commented Jan 2, 2025

The blur appeared too strong and spread out compared to typical drop shadow effects found in compositing window managers. Would you adjust the Gaussian kernel to create a tighter, more natural-looking shadow?

@weihsinyeh
Copy link
Collaborator Author

Do you want to directly reduce kernel size or use the gradient method to reduce kernel size?

@jserv
Copy link
Contributor

jserv commented Jan 3, 2025

Do you want to directly reduce kernel size or use the gradient method to reduce kernel size?

For window compositing, the blur effect should only be applied to the window decorator when creating drop shadows, not to the entire window. The kernel size shrinks.

@weihsinyeh
Copy link
Collaborator Author

Do all windows need to have the drop shadow feature? And only the window on the top layer will display the drop shadow.

@jserv
Copy link
Contributor

jserv commented Jan 3, 2025

Do all windows need to have the drop shadow feature? And only the window on the top layer will display the drop shadow.

Blur effects are only applied to the active window, using drop shadow to make it visually distinct.

@jserv
Copy link
Contributor

jserv commented Jan 4, 2025

It looks much better now.
blur

The latest update enforces blur effects on the active window, matching the standard behavior of compositing window managers. To enhance the visual depth, we should replace the right and bottom window borders with drop shadows, creating darker edges that give a more dimensional appearance. This means disabling the standard border rendering on those sides and implementing drop shadow effects instead.

Reference video: Apply Blur to xfce 4 Desktop Manager
picom is a compositor for X, and a fork of Compton.

@weihsinyeh
Copy link
Collaborator Author

This is the version that directly applies the shadow.
image
This is the version where the shadow has a gradient effect.
image
Is "Replace the right and bottom window borders with drop shadows, creating darker edges that give a more dimensional appearance" referring to one of these two?

@jserv
Copy link
Contributor

jserv commented Jan 4, 2025

Is "Replace the right and bottom window borders with drop shadows, creating darker edges that give a more dimensional appearance" referring to one of these two?

Yes, exactly.

@jserv jserv changed the title Add the Gaussian function to blur a pixmap Render drop show for active window Jan 4, 2025
@jserv jserv changed the title Render drop show for active window Render drop shadow for active window Jan 4, 2025
@jserv
Copy link
Contributor

jserv commented Jan 4, 2025

drop-shadow

The shadow should be aligned with the right and bottom borders, meaning the right shadow should be positioned at the top-right corner of the active window, while the bottom shadow should be placed at the bottom-left corner of the active window.

Action items:

  • Adjust the gradient effect region to make it narrower and more visually natural.
  • Make the drop shadow feature configurable via make config. If the feature is set, no bottom-right window decorator would appear.
  • Avoid changing function prototype of twin_window_create, making drop shadow independent from the configurations.
  • Ensure that drop shadow feature works with Pixman renderer instead of the built-in one.

src/draw-common.c Outdated Show resolved Hide resolved
include/twin_private.h Outdated Show resolved Hide resolved
Copy link

bito-code-review bot commented Feb 3, 2025

Code Review Agent Run #4bda41

Actionable Suggestions - 6
  • src/window.c - 1
    • Consider defining shadow config as constants · Line 385-386
  • apps/multi.c - 1
  • include/twin.h - 1
    • Consider initializing shadow boolean field · Line 204-204
  • src/draw-common.c - 2
  • configs/Kconfig - 1
    • Consider more descriptive config name · Line 74-78
Review Details
  • Files reviewed - 9 · Commit Range: ed659ba..ed659ba
    • apps/multi.c
    • configs/Kconfig
    • include/twin.h
    • include/twin_private.h
    • src/draw-common.c
    • src/image.c
    • src/pixmap.c
    • src/screen.c
    • src/window.c
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by Bito Logo

src/window.c Outdated Show resolved Hide resolved
apps/multi.c Outdated Show resolved Hide resolved
configs/Kconfig Outdated Show resolved Hide resolved
Copy link

bito-code-review bot commented Feb 4, 2025

Code Review Agent Run #32aeb7

Actionable Suggestions - 6
  • apps/multi.c - 2
  • configs/Kconfig - 2
    • Consider lower default shadow blur value · Line 76-78
    • Consider lower default shadow blur value · Line 76-78
  • src/pixmap.c - 1
    • Consider unconditional shadow field initialization · Line 48-50
  • src/window.c - 1
    • Consider validating blur radius config value · Line 389-391
Review Details
  • Files reviewed - 9 · Commit Range: 5f47a9a..5f47a9a
    • apps/multi.c
    • configs/Kconfig
    • include/twin.h
    • include/twin_private.h
    • src/draw-common.c
    • src/image.c
    • src/pixmap.c
    • src/screen.c
    • src/window.c
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by Bito Logo

apps/multi.c Outdated Show resolved Hide resolved
apps/multi.c Outdated Show resolved Hide resolved
include/twin_private.h Outdated Show resolved Hide resolved
Copy link

bito-code-review bot commented Feb 4, 2025

Code Review Agent Run #c6fdc0

Actionable Suggestions - 1
  • apps/multi.c - 1
Review Details
  • Files reviewed - 9 · Commit Range: 7f7310c..7f7310c
    • apps/multi.c
    • configs/Kconfig
    • include/twin.h
    • include/twin_private.h
    • src/draw-common.c
    • src/image.c
    • src/pixmap.c
    • src/screen.c
    • src/window.c
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by Bito Logo

Implement the twin_window_drop_shadow() function to handle the pixels
within the drop shadow area of the active window's pixel map. The drop
shadow effect of the window is only visible when the window is on the
top layer, ensuring the active window stands out visually.

Add the twin_stack_blur() function to implement Mario's Stack Blur
algorithm, which blurs the target pixel map. Additionally, create a blur
window in apps_blur() to show the effect of twin_stack_blur().

Implement the twin_shadow_border() function to create a darker border of
the active window that gives a more dimensional appearance.

The function twin_window_drop_shadow() will first apply the
twin_shadow_border() and then apply twin_stack_blur() to blur the darker
border.

Furthermore, implement the twin_cover() function to cover the target
pixels with the desired color.

Ref: https://melatonin.dev/blog/implementing-marios-stack-blur-15-times-in-cpp/
Close sysprog21#34

Signed-off-by: Wei-Hsin Yeh <[email protected]>
Copy link

bito-code-review bot commented Feb 4, 2025

Code Review Agent Run #ff7540

Actionable Suggestions - 6
  • apps/multi.c - 3
    • Consider validating coordinates before blur call · Line 340-340
    • Consider validating coordinates before blur call · Line 340-340
    • Consider preserving original alpha channel · Line 314-314
  • include/twin_private.h - 2
  • configs/Kconfig - 1
    • Consider consolidating shadow offset configs · Line 62-72
Additional Suggestions - 1
  • src/window.c - 1
    • Consider more descriptive constant name · Line 21-21
Review Details
  • Files reviewed - 9 · Commit Range: d493323..d493323
    • apps/multi.c
    • configs/Kconfig
    • include/twin.h
    • include/twin_private.h
    • src/draw-common.c
    • src/image.c
    • src/pixmap.c
    • src/screen.c
    • src/window.c
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by Bito Logo

@jserv jserv merged commit f09d14b into sysprog21:main Feb 4, 2025
3 checks passed
@jserv
Copy link
Contributor

jserv commented Feb 4, 2025

Thank @weihsinyeh for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants